Stencil v2.16.0 introduced a regression, the symptoms of which are
described here: #3403. Git
bisect showed that this commit was the culprit:
cca8951.

What was the regression?
------------------------

What the user observed is that after upgrading from Stencil version
2.15.2 to version 2.16.0 the build output changed. With 2.15.2, when
calling stencil like so:

```
stencil build --docs --dev --esm
```

the build output looked something like this:

```
dist
├── cjs
├── collection
│   ├── components
│   ├── test
│   └── utils
├── esm
│   └── polyfills
├── loader
├── stencil-one
└── types
```

Whereas after upgrading to 2.16.0 the output looked something like this:

```
dist
├── stencil-one
└── types
```

Why did the built output change?
--------------------------------

Stencil previously explicitly supported a `--esm` boolean command-line
flag. Before [the change to remove the legacy
compiler](63595bc)
this flag was used to control whether the output would be ESModules or
not (I _think_).

The flag was removed from the supported arguments list in [that same
commit](63595bc#diff-e6a96bc13cd5c047a16a1888b27e88af52903e89848de68b0072d32ea11b1208L196),
however some code that implicitly depended on the argument remained in
the codebase, and it was eventually (partially) [added back to the
codebase later on](#2339) but,
although a change was made to `src/cli/parse-flags.ts` to support
parsing the flag, it was *not* added to the `ConfigFlags` interface,
which is intended to capture the names and types of all CLI arguments.

Related to this history of the flag being added and removed from
Stencil, on `src/compiler/config/validate-config.ts` there was, prior to
the bad commit which introduced this regression, a call to the
`setBooleanConfig` helper that looked like this:

```ts
setBooleanConfig(config, 'buildDist', 'esm', !config.devMode || config.buildEs5);
```

This line creates an undocumented dependency on the `--esm` command-line
flag, where it's presence or absence changes what the value of
`config.buildDist` will be.

This happens because of the implementation of `setBooleanConfig`, where
if a value is passed for the third positional argument it will look at
the command-line arguments passed to the Stencil CLI in order to
find a value to set on the `Config` object. The relevant code looks
like this:

```ts
export const setBooleanConfig = <K extends keyof d.Config>(
  config: d.UnvalidatedConfig,
  configName: (K & keyof d.ConfigFlags) | K,
  flagName: keyof d.ConfigFlags | null,
  defaultValue: d.Config[K]
) => {
  if (flagName) {
    const flagValue = config.flags?.[flagName];
    if (isBoolean(flagValue)) {
      config[configName] = flagValue;
    }
  }
```

The [commit that introduced the
regression](cca8951#diff-53c120bef3205f89229fbe98a5aadfcb51012ead144729ade2fc597c1e9ad5e9R64)
changed the above call to `setBooleanConfig` to this:

```ts
setBooleanConfig(config, 'buildDist', null, !config.devMode || config.buildEs5);
```

The user observed the regression because they were using the `--esm`
command-line flag with their project, so on any stencil version <=
2.15.2 the presence of that flag was causing `config.buildDist` to be
set to `true`, resulting in the expected build output above. In 2.16.0
the behavior of the `--esm` flag was broken by change from `"esm"` to
`null`.

Why was the change introduced?
------------------------------

This change was introduced because `"esm"` was not listed as a supported
command-line argument on the `ConfigFlags` interface and the third
position argument to `setBooleanConfig` has type `keyof d.ConfigFlags`,
so the change to `null` was in line with what `setBooleanConfig` expects
for its arguments. The commit author (me!) was unsure of whether to make
this change or not, and there was [some discussion on the
PR](#3335 (comment)).
Light testing did not turn up the regression because we weren't
using the `--esm` flag.

What is the fix?
----------------

The fix is to add `esm` to the `ConfigFlags` interface and to revert the
bad line in `src/compiler/config/validate-config.ts`. This will allow
anyone who is currently using the `--esm` flag to keep doing so.

This fix does not deal with whether or not we want to continue
supporting the `--esm` flag — it's possible that a larger refactor to
how the `buildDist` variable is used throughout the codebase is in order
— but doing this will unblock any users who are currently depending on
Stencil's previous, undocumented behavior.

I think this is the right fix for now, however, because an API change
like this should not happen on a minor release, so the behavior of the
compiler changing in this way should be viewed as a bug and we should
take the steps to restore the old behavior.

Any larger lessons learned?
---------------------------

Part of the reason for the regression was that the CLI argument parsing
code does not have a type level contract with the interface into which
the CLI args are parsed. In particular, there are lists of supported CLI
flags in `src/cli/parse-flags.ts` but these have _no connection_ with
the `ConfigFlags` interface, which describes the shape of the object
into which they are going to be shoved after their values are parsed.

This means that although we have an interface that sort of _claims_, at
least implicitly, because of how it's named, to enumerate the possible /
allowed CLI arguments, we aren't actually utilizing the type system to
enforce a tighter contract between the parsing code and that interface,
so there is a possibility of the arguments declared in one or the other
of these two places (the `parse-flags.ts` code or the `ConfigFlags`
interface) drifting from the other.

To address that root cause I think we should implement something to
declare these properties in one place. For instance, we could declare a
`ReadonlyArray` of Boolean flags, another of String flags, and then use
types derived from those as keys for the `ConfigFlags` argument.

Additionally, several of the commits in the history of the repository
which had to do with this flag have no discussion, documentation, or
context provided for the changes.

Stencil v2.16.0 introduced a regression, the symptoms of which are
described here: #3403. Git
bisect showed that this commit was the culprit:
cca8951.

What was the regression?
------------------------

What the user observed is that after upgrading from Stencil version
2.15.2 to version 2.16.0 the build output changed. With 2.15.2, when
calling stencil like so:

```
stencil build --docs --dev --esm
```

the build output looked something like this:

```
dist
├── cjs
├── collection
│   ├── components
│   ├── test
│   └── utils
├── esm
│   └── polyfills
├── loader
├── stencil-one
└── types
```

Whereas after upgrading to 2.16.0 the output looked something like this:

```
dist
├── stencil-one
└── types
```

Why did the built output change?
--------------------------------

Stencil previously explicitly supported a `--esm` boolean command-line
flag. Before [the change to remove the legacy
compiler](63595bc)
this flag was used to control whether the output would be ES modules or
not (I _think_).

The flag was removed from the supported arguments list in [that same
commit](63595bc#diff-e6a96bc13cd5c047a16a1888b27e88af52903e89848de68b0072d32ea11b1208L196),
however some code that implicitly depended on the argument remained in
the codebase, and it was eventually (partially) [added back to the
codebase later on](#2339) but,
although a change was made to `src/cli/parse-flags.ts` to support
parsing the flag, it was *not* added to the `ConfigFlags` interface,
which is intended to capture the names and types of all CLI arguments.

Related to this history of the flag being added and removed from
Stencil, on `src/compiler/config/validate-config.ts` there was, prior to
the bad commit which introduced this regression, a call to the
`setBooleanConfig` helper that looked like this:

```ts
setBooleanConfig(config, 'buildDist', 'esm', !config.devMode || config.buildEs5);
```

This line creates an undocumented dependency on the `--esm` command-line
flag, where it's presence or absence changes what the value of
`config.buildDist` will be.

This happens because of the implementation of `setBooleanConfig`, where
if a value is passed for the third positional argument it will look at
the command-line arguments passed to the Stencil CLI in order to
find a value to set on the `Config` object. The relevant code looks
like this:

```ts
export const setBooleanConfig = <K extends keyof d.Config>(
  config: d.UnvalidatedConfig,
  configName: (K & keyof d.ConfigFlags) | K,
  flagName: keyof d.ConfigFlags | null,
  defaultValue: d.Config[K]
) => {
  if (flagName) {
    const flagValue = config.flags?.[flagName];
    if (isBoolean(flagValue)) {
      config[configName] = flagValue;
    }
  }
```

The [commit that introduced the
regression](cca8951#diff-53c120bef3205f89229fbe98a5aadfcb51012ead144729ade2fc597c1e9ad5e9R64)
changed the above call to `setBooleanConfig` to this:

```ts
setBooleanConfig(config, 'buildDist', null, !config.devMode || config.buildEs5);
```

The user observed the regression because they were using the `--esm`
command-line flag with their project, so on any stencil version <=
2.15.2 the presence of that flag was causing `config.buildDist` to be
set to `true`, resulting in the expected build output above. In 2.16.0
the behavior of the `--esm` flag was broken by change from `"esm"` to
`null`.

Why was the change introduced?
------------------------------

This change was introduced because `"esm"` was not listed as a supported
command-line argument on the `ConfigFlags` interface and the third
position argument to `setBooleanConfig` has type `keyof d.ConfigFlags`,
so the change to `null` was in line with what `setBooleanConfig` expects
for its arguments. The commit author (me!) was unsure of whether to make
this change or not, and there was [some discussion on the
PR](#3335 (comment)).
Light testing did not turn up the regression because we weren't
using the `--esm` flag.

What is the fix?
----------------

The fix is to add `esm` to the `ConfigFlags` interface and to revert the
bad line in `src/compiler/config/validate-config.ts`. This will allow
anyone who is currently using the `--esm` flag to keep doing so.

This fix does not deal with whether or not we want to continue
supporting the `--esm` flag — it's possible that a larger refactor to
how the `buildDist` variable is used throughout the codebase is in order
— but doing this will unblock any users who are currently depending on
Stencil's previous, undocumented behavior.

I think this is the right fix for now, however, because an API change
like this should not happen on a minor release, so the behavior of the
compiler changing in this way should be viewed as a bug and we should
take the steps to restore the old behavior.

Any larger lessons learned?
---------------------------

Part of the reason for the regression was that the CLI argument parsing
code does not have a type level contract with the interface into which
the CLI args are parsed. In particular, there are lists of supported CLI
flags in `src/cli/parse-flags.ts` but these have _no connection_ with
the `ConfigFlags` interface, which describes the shape of the object
into which they are going to be shoved after their values are parsed.

This means that although we have an interface that sort of _claims_, at
least implicitly, because of how it's named, to enumerate the possible /
allowed CLI arguments, we aren't actually utilizing the type system to
enforce a tighter contract between the parsing code and that interface,
so there is a possibility of the arguments declared in one or the other
of these two places (the `parse-flags.ts` code or the `ConfigFlags`
interface) drifting from the other.

To address that root cause I think we should implement something to
declare these properties in one place. For instance, we could declare a
`ReadonlyArray` of Boolean flags, another of String flags, and then use
types derived from those as keys for the `ConfigFlags` argument.

Additionally, several of the commits in the history of the repository
which had to do with this flag have no discussion, documentation, or
context provided for the changes.

Stencil v2.16.0 introduced a regression, the symptoms of which are
described here: #3403. Git
bisect showed that this commit was the culprit:
cca8951.

What was the regression?
------------------------

What the user observed is that after upgrading from Stencil version
2.15.2 to version 2.16.0 the build output changed. With 2.15.2, when
calling stencil like so:

```
stencil build --docs --dev --esm
```

the build output looked something like this:

```
dist
├── cjs
├── collection
│   ├── components
│   ├── test
│   └── utils
├── esm
│   └── polyfills
├── loader
├── stencil-one
└── types
```

Whereas after upgrading to 2.16.0 the output looked something like this:

```
dist
├── stencil-one
└── types
```

Why did the built output change?
--------------------------------

Stencil previously explicitly supported a `--esm` boolean command-line
flag. Before [the change to remove the legacy
compiler](63595bc)
this flag was used to control whether the output would be ES modules or
not (I _think_).

The flag was removed from the supported arguments list in [that same
commit](63595bc#diff-e6a96bc13cd5c047a16a1888b27e88af52903e89848de68b0072d32ea11b1208L196),
however some code that implicitly depended on the argument remained in
the codebase, and it was eventually (partially) [added back to the
codebase later on](#2339) but,
although a change was made to `src/cli/parse-flags.ts` to support
parsing the flag, it was *not* added to the `ConfigFlags` interface,
which is intended to capture the names and types of all CLI arguments.

Related to this history of the flag being added and removed from
Stencil, on `src/compiler/config/validate-config.ts` there was, prior to
the bad commit which introduced this regression, a call to the
`setBooleanConfig` helper that looked like this:

```ts
setBooleanConfig(config, 'buildDist', 'esm', !config.devMode || config.buildEs5);
```

This line creates an undocumented dependency on the `--esm` command-line
flag, where it's presence or absence changes what the value of
`config.buildDist` will be.

This happens because of the implementation of `setBooleanConfig`, where
if a value is passed for the third positional argument it will look at
the command-line arguments passed to the Stencil CLI in order to
find a value to set on the `Config` object. The relevant code looks
like this:

```ts
export const setBooleanConfig = <K extends keyof d.Config>(
  config: d.UnvalidatedConfig,
  configName: (K & keyof d.ConfigFlags) | K,
  flagName: keyof d.ConfigFlags | null,
  defaultValue: d.Config[K]
) => {
  if (flagName) {
    const flagValue = config.flags?.[flagName];
    if (isBoolean(flagValue)) {
      config[configName] = flagValue;
    }
  }
```

The [commit that introduced the
regression](cca8951#diff-53c120bef3205f89229fbe98a5aadfcb51012ead144729ade2fc597c1e9ad5e9R64)
changed the above call to `setBooleanConfig` to this:

```ts
setBooleanConfig(config, 'buildDist', null, !config.devMode || config.buildEs5);
```

The user observed the regression because they were using the `--esm`
command-line flag with their project, so on any stencil version <=
2.15.2 the presence of that flag was causing `config.buildDist` to be
set to `true`, resulting in the expected build output above. In 2.16.0
the behavior of the `--esm` flag was broken by change from `"esm"` to
`null`.

Why was the change introduced?
------------------------------

This change was introduced because `"esm"` was not listed as a supported
command-line argument on the `ConfigFlags` interface and the third
position argument to `setBooleanConfig` has type `keyof d.ConfigFlags`,
so the change to `null` was in line with what `setBooleanConfig` expects
for its arguments. The commit author (me!) was unsure of whether to make
this change or not, and there was [some discussion on the
PR](#3335 (comment)).
Light testing did not turn up the regression because we weren't
using the `--esm` flag.

What is the fix?
----------------

The fix is to add `esm` to the `ConfigFlags` interface and to revert the
bad line in `src/compiler/config/validate-config.ts`. This will allow
anyone who is currently using the `--esm` flag to keep doing so.

This fix does not deal with whether or not we want to continue
supporting the `--esm` flag — it's possible that a larger refactor to
how the `buildDist` variable is used throughout the codebase is in order
— but doing this will unblock any users who are currently depending on
Stencil's previous, undocumented behavior.

I think this is the right fix for now, however, because an API change
like this should not happen on a minor release, so the behavior of the
compiler changing in this way should be viewed as a bug and we should
take the steps to restore the old behavior.

Any larger lessons learned?
---------------------------

Part of the reason for the regression was that the CLI argument parsing
code does not have a type level contract with the interface into which
the CLI args are parsed. In particular, there are lists of supported CLI
flags in `src/cli/parse-flags.ts` but these have _no connection_ with
the `ConfigFlags` interface, which describes the shape of the object
into which they are going to be shoved after their values are parsed.

This means that although we have an interface that sort of _claims_, at
least implicitly, because of how it's named, to enumerate the possible /
allowed CLI arguments, we aren't actually utilizing the type system to
enforce a tighter contract between the parsing code and that interface,
so there is a possibility of the arguments declared in one or the other
of these two places (the `parse-flags.ts` code or the `ConfigFlags`
interface) drifting from the other.

To address that root cause I think we should implement something to
declare these properties in one place. For instance, we could declare a
`ReadonlyArray` of Boolean flags, another of String flags, and then use
types derived from those as keys for the `ConfigFlags` argument.

Additionally, several of the commits in the history of the repository
which had to do with this flag have no discussion, documentation, or
context provided for the changes.